docs: clarify frontmatter hash is stale-lock detection, not tamper protection#24198
Merged
docs: clarify frontmatter hash is stale-lock detection, not tamper protection#24198
Conversation
…ction Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c0417ee1-2423-45b1-ab1f-a7c3c2adbc05 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
April 3, 2026 00:45
View session
Collaborator
|
@lpcox fyi |
pelikhan
approved these changes
Apr 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Clarifies that the frontmatter hash is intended to detect stale lock files (lock out of sync with source) rather than providing tamper protection/security guarantees.
Changes:
- Updates the frontmatter hash specification to describe the feature as stale-lock detection and explicitly disclaim tamper protection.
- Rewords the workflow lock check script’s docstring and user-facing logs/warnings away from “integrity/security” language toward “stale/unverifiable” language.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/src/content/docs/reference/frontmatter-hash-specification.md | Reframes purpose + security considerations to emphasize stale-lock detection and clarify lack of tamper protection. |
| actions/setup/js/check_workflow_timestamp_api.cjs | Updates module documentation and warning/log messaging to align with stale-lock detection framing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
195
to
199
| if (!hashComparison) { | ||
| // Could not compute hash - be conservative and fail | ||
| core.warning("Could not compare frontmatter hashes - assuming lock file is outdated"); | ||
| const warningMessage = `Lock file '${lockFilePath}' integrity check failed! Could not verify frontmatter hash for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`; | ||
| const warningMessage = `Lock file '${lockFilePath}' is outdated or unverifiable! Could not verify frontmatter hash for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`; | ||
|
|
There was a problem hiding this comment.
The failure/warning message text here has changed (no longer includes "integrity check failed"), but the unit tests still assert on the old substring (e.g., actions/setup/js/check_workflow_timestamp_api.test.cjs lines ~237, ~248, ~350, etc.). Please update those expectations to match the new wording, otherwise CI will fail.
See below for a potential fix:
const warningMessage = `Lock file '${lockFilePath}' is outdated or unverifiable! Frontmatter integrity check failed for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`;
let summary = core.summary
.addRaw("### ⚠️ Workflow Lock File Warning\n\n")
.addRaw("**WARNING**: Could not verify whether lock file is up to date. Frontmatter integrity check failed.\n\n")
|
|
||
| The frontmatter hash provides: | ||
| 1. **Change detection**: Verify that workflow configuration has not changed between compilation and execution | ||
| 1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the `.md` file without recompiling) |
There was a problem hiding this comment.
This purpose statement implies that any edit to the .md workflow file will be detected as a stale lock, but the spec explicitly excludes markdown body content from the hash (see the Excluded Fields list). Consider narrowing the wording to “after editing the workflow frontmatter (or imported frontmatter) without recompiling” to avoid overstating what the hash detects.
Suggested change
| 1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the `.md` file without recompiling) | |
| 1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the workflow frontmatter or imported frontmatter without recompiling) |
Comment on lines
+8
to
+10
| * the workflow was edited without recompiling the lock file. It does not | ||
| * provide tamper protection — use code review to guard against intentional | ||
| * modifications. |
There was a problem hiding this comment.
The docstring says it detects cases where “the workflow was edited without recompiling the lock file,” but the comparison is based on the frontmatter hash (not the full markdown workflow content). To avoid implying broader coverage than implemented, consider rephrasing to “frontmatter (and imported frontmatter) was edited without recompiling.”
Suggested change
| * the workflow was edited without recompiling the lock file. It does not | |
| * provide tamper protection — use code review to guard against intentional | |
| * modifications. | |
| * frontmatter (and imported frontmatter) was edited without recompiling the | |
| * lock file. It does not provide tamper protection — use code review to guard | |
| * against intentional modifications. |
lpcox
added a commit
that referenced
this pull request
Apr 3, 2026
Tests asserted 'integrity check failed' but the error message was changed to 'is outdated or unverifiable' in #24198. Updated all 6 assertions to match the current production error format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox
added a commit
that referenced
this pull request
Apr 3, 2026
…24221) * fix: clear stale GH_HOST and remove false fork PR detection Two bugs interacted to cause PR checkout failures on github.com repos: 1. configure_gh_for_ghe.sh: For github.com, the script returned early without clearing GH_HOST. A stale value (e.g., localhost:18443 from a prior DIFC proxy run) persisted, causing 'gh pr checkout' to fail with 'none of the git remotes correspond to GH_HOST'. Now explicitly unsets stale GH_HOST values on the github.com path. 2. pr_helpers.cjs: detectForkPR() checked head.repo.fork to detect fork PRs, but this flag indicates whether the *repository itself* is a fork (common in OSS), not whether the PR is cross-repo. This caused false positives that forced the slow 'gh pr checkout' path (which depends on GH_HOST) instead of fast 'git fetch' (which doesn't). Removed the fork flag check; now only uses full_name comparison. Fixes #24208 Closes #24217 Closes #24218 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: update push_to_pr_branch test for corrected fork detection The test 'should detect fork PR via fork flag and fail early' assumed that head.repo.fork=true with same full_name meant a cross-repo fork PR. After fixing detectForkPR() to only use full_name comparison (#24208), same-repo PRs in forked repositories are correctly treated as non-fork, allowing push to proceed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: update checkout_pr_branch tests for corrected fork detection The mock detectForkPR() in checkout_pr_branch.test.cjs replicated the old fork-flag-based logic, making tests inconsistent with production. Updated the mock to match the corrected full_name-only comparison and updated assertions to verify that same-repo PRs in forked repositories use the fast git fetch path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: fix check_workflow_timestamp_api tests for updated error messages Tests asserted 'integrity check failed' but the error message was changed to 'is outdated or unverifiable' in #24198. Updated all 6 assertions to match the current production error format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox
added a commit
that referenced
this pull request
Apr 3, 2026
Tests asserted 'integrity check failed' but the error message was changed to 'is outdated or unverifiable' in #24198. Updated all 6 assertions to match the current production error format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The frontmatter hash was described using "integrity" and "security" language implying tamper protection — which it cannot provide, since anyone with repo write access can modify both
.mdand.lock.ymltogether. Its actual purpose is detecting stale lock files (i.e. the workflow was edited without recompiling).Changes
frontmatter-hash-specification.mdcheck_workflow_timestamp_api.cjscore.infolog and warning messages: replaced "integrity check failed" with "could not verify whether lock file is up to date"